use afit and remove async-trait #2308

lz1998 · 2023-11-10T07:56:49Z

Motivation

1.75.0 will be stable on: 28 December, 2023.

Remove async-trait and use AFIT(Async Fn In Trait) and RPITIT(Return Position Impl Trait In Traits).

Solution

davidpdrsn · 2023-11-10T08:19:27Z

Are you able to make it actually compile?

I'm not totally up to date on the specifics of async fn in traits but last I heard there were some shortcomings around declaring/propagating Send bounds. That probably will be a deal breaker for axum.

jplatte · 2023-11-10T08:51:34Z

The situation is this: You still can't write a generic function like fn foo<T: FromRequest>() where T::foo(): Send to say you need the returned future to be Send. However, async_trait was already enforcing Sendness for all trait implementations, and that is still possible by using fn from_request(...) -> impl Future<Output = Self> + Send in the trait declaration, such that callers can rely on all implementations fulfilling that. Implementations can then use async fn sugar.

As such, I think it makes sense to get this in for the next breaking release. It removes a bunch of indirection, and allows auto traits other than Send to be inferred in non-generic contexts. It should also make the documentation a little easier to read, and might even allow a few projects to get rid of syn & quote in their dependency trees (turbo.fish is really close, other than axum's async-trait dependency there is only tower's pin-project dependency which is still waiting on a breaking-change release though, unless the decision to not merge my PR removing it by introducing a few lines of unsafe is revisited).

jplatte · 2023-11-10T08:52:38Z

As for this PR, it introduces a number of trait declarations that don't use + Send after -> impl Future and I think that should be changed, even if there is no code that relies on the Sendness of the returned futures in generic code inside axum.

davidpdrsn · 2023-11-10T10:38:59Z

Thats great news! 🎉

lz1998 · 2023-11-10T18:28:40Z

Are you able to make it actually compile?

I'm not totally up to date on the specifics of async fn in traits but last I heard there were some shortcomings around declaring/propagating Send bounds. That probably will be a deal breaker for axum.

It can be compiled on nightly version.

rustup default nightly

lz1998 · 2023-11-13T11:57:55Z

As for this PR, it introduces a number of trait declarations that don't use + Send after -> impl Future and I think that should be changed, even if there is no code that relies on the Sendness of the returned futures in generic code inside axum.

Why is + Send nessesary? It can be compiled without + Send.

jplatte

Thanks! Have now reviewed everything and it looks pretty good, just some minor things could still be improved.

axum-core/src/ext_traits/request.rs

axum-core/src/ext_traits/request_parts.rs

jplatte · 2023-11-13T12:28:34Z

axum-extra/src/either.rs

@@ -247,12 +242,12 @@ macro_rules! impl_traits_for_either {

            async fn from_request_parts(parts: &mut Parts, state: &S) -> Result<Self, Self::Rejection> {
                $(
-                    if let Ok(value) = FromRequestParts::from_request_parts(parts, state).await {
+                    if let Ok(value) = $ident::from_request_parts(parts, state).await {


Why did you change this? It means that an inherent from_request_parts would be called here if it exists in addition to the one from a FromRequestParts impl, which I don't think is right.

Because an error occurs if I don't change this.

error[E0282]: type annotations needed --> axum-extra/src/either.rs:250:17 | 250 | FromRequestParts::from_request_parts(parts, state).await.map(Self::$last) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type ... 275 | impl_traits_for_either!(Either8 => [E1, E2, E3, E4, E5, E6, E7], E8); | -------------------------------------------------------------------- in this macro invocation | = note: this error originates in the macro `impl_traits_for_either` (in Nightly builds, run with -Z macro-backtrace for more info) For more information about this error, try `rustc --explain E0282`. error: could not compile `axum-extra` (lib) due to 7 previous errors

I have changed it to this.

async fn from_request_parts(parts: &mut Parts, state: &S) -> Result<Self, Self::Rejection> { $( if let Ok(value) = <$ident as FromRequestParts<S>>::from_request_parts(parts, state).await { return Ok(Self::$ident(value)); } )* <$last as FromRequestParts<S>>::from_request_parts(parts, state).await.map(Self::$last) }

jplatte · 2023-11-13T12:33:00Z

Why is + Send nessesary? It can be compiled without + Send.

Because it's impossible for generic functions to declare that they accept any implementer of a given trait where one or all of its async methods return a Send future.

jplatte

Thanks! I think we can merge this once RPITIT & AFIT hit stable! 🎉

lz1998 · 2023-12-16T08:44:07Z

I have rebased it to axum 0.7

lz1998 · 2023-12-29T09:54:53Z

There are still a lot of macros need to be fixed.

::axum::extract::FromRequest::from_request

should be changed to

<#field_ty as ::axum::extract::FromRequest<#trait_generics, _>>::from_request // the second generic type is private::ViaParts

---- debug_handler::ui stdout ----
thread 'debug_handler::ui' panicked at /Users/lz1998/.cargo/registry/src/rsproxy.cn-8f6827c7555bfaf8/trybuild-1.0.86/src/run.rs:101:13:
6 of 29 tests failed

---- from_request::ui stdout ----
thread 'from_request::ui' panicked at /Users/lz1998/.cargo/registry/src/rsproxy.cn-8f6827c7555bfaf8/trybuild-1.0.86/src/run.rs:101:13:
15 of 58 tests failed



error[E0282]: type annotations needed
  --> src/main.rs:22:11
   |
22 |     host: Result<headers::Host, TypedHeaderRejection>,
   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type

lz1998 · 2023-12-29T17:03:06Z

@davidpdrsn It's hard to solve the problem of macros because of the second generic type private::ViaParts, can you help to solve this?

davidpdrsn · 2023-12-29T17:08:57Z

Sure I'll try and take a look!

lz1998 · 2023-12-29T17:22:12Z

Sure I'll try and take a look!

I have rebased it to the main branch.

davidpdrsn · 2023-12-29T19:14:30Z

Thanks for all your work on this by the way ❤️

gabibbo97 · 2024-01-07T21:49:20Z

Just to add on this, if someone was wondering about the performance implications.

(Tested with --release, lto = "thin", and codegen-units = 1)

I am comparing with the following Cargo.toml

[package]
name = "bench"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[profile.release]
lto = "thin"
codegen-units = 1

[dependencies]
tokio = { version = "1", features = [ "full" ] }

# This PR
axum = { git = "https://github.com/lz1998/axum.git", branch = "main" }

# Upstream
#axum = { git = "https://github.com/tokio-rs/axum.git", rev = "c1c917092daeb91456b41510b629c4ba9763b2b6" }

and this main.rs

use axum::{
    routing::get,
    Router,
};

#[tokio::main]
async fn main() {
    // build our application with a single route
    let app = Router::new()
        .route("/", get(|| async { "Hello, World!" }));

    // run our app with hyper, listening globally on port 3000
    let listener = tokio::net::TcpListener::bind("0.0.0.0:3000").await.unwrap();
    axum::serve(listener, app).await.unwrap();
}

Baseline results

# --release lto = "thin" codegen-units = 1
> wrk -t1 -c1 --latency http://127.0.0.1:3000    
Running 10s test @ http://127.0.0.1:3000
  1 threads and 1 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    12.35us    5.98us 330.00us   95.38%
    Req/Sec    80.57k    10.08k   88.10k    88.00%
  Latency Distribution
     50%   11.00us
     75%   12.00us
     90%   14.00us
     99%   33.00us
  800650 requests in 10.00s, 99.26MB read
Requests/sec:  80064.18
Transfer/sec:      9.93MB

> wrk -t16 -c1000 --latency http://127.0.0.1:3000
Running 10s test @ http://127.0.0.1:3000
  16 threads and 1000 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.04ms    1.16ms  39.22ms   88.95%
    Req/Sec    71.86k     8.35k  114.12k    74.25%
  Latency Distribution
     50%  608.00us
     75%    1.18ms
     90%    2.35ms
     99%    5.55ms
  11481372 requests in 10.08s, 1.39GB read
Requests/sec: 1139313.22
Transfer/sec:    141.25MB

> ls -lb target/release
-rwxr-xr-x.  2 giacomo giacomo 8417072 Jan  7 22:40 bench

PR results

# --release lto = "thin" codegen-units = 1
> wrk -t1 -c1 --latency http://127.0.0.1:3000    
Running 10s test @ http://127.0.0.1:3000
  1 threads and 1 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    11.34us    4.51us 588.00us   98.88%
    Req/Sec    86.17k     5.20k   88.44k    98.02%
  Latency Distribution
     50%   11.00us
     75%   11.00us
     90%   12.00us
     99%   16.00us
  865967 requests in 10.10s, 107.36MB read
Requests/sec:  85737.63
Transfer/sec:     10.63MB

> wrk -t16 -c1000 --latency http://127.0.0.1:3000
Running 10s test @ http://127.0.0.1:3000
  16 threads and 1000 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.02ms    1.12ms  18.49ms   88.87%
    Req/Sec    72.55k     7.59k  100.27k    67.81%
  Latency Distribution
     50%  604.00us
     75%    1.16ms
     90%    2.30ms
     99%    5.51ms
  11593246 requests in 10.06s, 1.40GB read
Requests/sec: 1152503.62
Transfer/sec:    142.88MB

> ls -lb target/release
-rwxr-xr-x.  2 giacomo giacomo 8417080 Jan  7 22:38 bench

As you can see, latency and throughput improves with almost no difference in binary size

Also interesting that the P99 latency for -c1 is halved

rbruenig · 2024-07-14T08:58:27Z

@lz1998 @jplatte RPITIT & AFIT (Rust 1.75) have been stable for more than 6 months now. Is there a chance that this can be merged soon? If theres any work remaining I'd like to help.

jplatte · 2024-09-20T21:30:34Z

There is a chance, though getting a release out is a lot more involved (due to the matchit upgrade, but also a few other planned changes).

Anyways.. @lz1998 I just fixed merge conflicts for you, would you mind taking another look at the current CI failures?

lz1998 · 2024-09-24T03:26:15Z

There is a chance, though getting a release out is a lot more involved (due to the matchit upgrade, but also a few other planned changes).

Anyways.. @lz1998 I just fixed merge conflicts for you, would you mind taking another look at the current CI failures?

The commented line will cause an error, because the second generic type private::ViaRequest is private. Can you help to solve this? @jplatte
https://github.com/lz1998/axum/blob/main/axum-macros/src/lib.rs#L162

use axum::extract::{FromRequest, Request};
use axum_extra::{
    TypedHeader,
    headers::{UserAgent},
};

struct MyExtractor {}
impl<S: Send + Sync> FromRequest<S> for MyExtractor {
    type Rejection = ();

    async fn from_request(req: Request, state: &S) -> Result<Self, Self::Rejection> {
        // let _ = <TypedHeader::<UserAgent> as FromRequest<S>>::from_request(req, state).await;
        let _ = TypedHeader::<UserAgent>::from_request(req, state).await;
        Ok(MyExtractor {})
    }
}


#[tokio::main]
async fn main() {}

error[E0277]: the trait bound `TypedHeader<UserAgent>: FromRequest<S>` is not satisfied
  --> src/main.rs:18:18
   |
18 |         let _ = <TypedHeader::<UserAgent> as FromRequest<S>>::from_request(req, state).await;
   |                  ^^^^^^^^^^^^^^^^^^^^^^^^ the trait `FromRequest<S>` is not implemented for `TypedHeader<UserAgent>`
   |
   = note: Function argument is not a valid axum extractor. 
           See `https://docs.rs/axum/0.7/axum/extract/index.html` for details
   = help: the following other types implement trait `FromRequest<S, M>`:
             (T1, T2)
             (T1, T2, T3)
             (T1, T2, T3, T4)
             (T1, T2, T3, T4, T5)
             (T1, T2, T3, T4, T5, T6)
             (T1, T2, T3, T4, T5, T6, T7)
             (T1, T2, T3, T4, T5, T6, T7, T8)
             (T1, T2, T3, T4, T5, T6, T7, T8, T9)
           and 20 others

jplatte · 2024-09-24T07:57:31Z

Yeah, in that case you wouldn't use T::from_request, you'd use T::from_request_parts or RequestExt::extract_parts.

jplatte · 2024-09-27T21:08:59Z

Oh, that code isn't what we actually have as an example whatever, it's macro-generated? I don't see how this would be a new problem though.. Will look at what changed in the macro code.

axum-macros/src/from_request.rs

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>

jplatte · 2024-09-28T10:29:21Z

Small tip, instead of accepting every suggestion individually, you can go to the files tab of the PR and batch them up into a single commit 😉

Also, I opened #2943 for the MSRV bump that's needed for this.

HigherOrderLogic · 2024-10-01T07:11:21Z

Is it possible to get a beta release with this patch included?

jplatte · 2024-10-01T07:16:07Z

I was planning to do a pre-release soon, yes. I'll probably wait until we have merged the matchit upgrade though.

jplatte · 2024-10-05T14:26:56Z

This is out as part of the latest set of alpha releases :)

jplatte reviewed Nov 13, 2023

View reviewed changes

jplatte approved these changes Nov 14, 2023

View reviewed changes

matze mentioned this pull request Nov 15, 2023

Drop async_trait hyperium/tonic#1556

Open

masnagam mentioned this pull request Nov 25, 2023

Remove async-trait mirakc/mirakc#1335

Open

lz1998 force-pushed the main branch from d715e22 to d2ab025 Compare December 16, 2023 08:39

lz1998 force-pushed the main branch from 8f3d3af to 22ada5d Compare December 28, 2023 09:02

lz1998 added 2 commits December 30, 2023 01:09

remove async_trait

a160f98

fix

3788043

lz1998 force-pushed the main branch from 97c873a to 3788043 Compare December 29, 2023 17:21

davidpdrsn added this to the 0.8 milestone Dec 30, 2023

davidpdrsn added the breaking change A PR that makes a breaking change. label Dec 30, 2023

Merge branch 'main' into main

361632f

carloskiki mentioned this pull request Sep 21, 2024

tower-sessions-core crate rework maxcountryman/tower-sessions#218

Closed

Update main.rs

7755d1e

Update lib.rs

fc470ce

jplatte reviewed Sep 27, 2024

View reviewed changes

lz1998 and others added 6 commits September 28, 2024 18:24

Update axum-macros/src/from_request.rs

8b76b09

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>

Update axum-macros/src/from_request.rs

b46976f

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>

Update axum-macros/src/from_request.rs

0e0ad98

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>

Update axum-macros/src/from_request.rs

e305bbb

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>

Update axum-macros/src/from_request.rs

aac67f1

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>

Update axum-macros/src/from_request.rs

d9f2035

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>

lz1998 and others added 7 commits September 28, 2024 18:40

revert trait_generics

133b8ab

Merge branch 'main' into main

2077c89

fix

7b5cf34

Fix missing qualification

11c702c

Fix remaining macro compile errors

b9020db

Update expected trybuild output

42703db

Merge branch 'main' into lz1998

8f64cc0

jplatte enabled auto-merge (squash) September 28, 2024 21:25

jplatte merged commit 19101f6 into tokio-rs:main Sep 28, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use afit and remove async-trait #2308

use afit and remove async-trait #2308

lz1998 commented Nov 10, 2023 •

edited

Loading

davidpdrsn commented Nov 10, 2023

jplatte commented Nov 10, 2023

jplatte commented Nov 10, 2023

davidpdrsn commented Nov 10, 2023

lz1998 commented Nov 10, 2023

lz1998 commented Nov 13, 2023

jplatte left a comment

jplatte Nov 13, 2023

lz1998 Nov 14, 2023

lz1998 Nov 14, 2023

jplatte commented Nov 13, 2023

jplatte left a comment

lz1998 commented Dec 16, 2023

lz1998 commented Dec 29, 2023 •

edited

Loading

lz1998 commented Dec 29, 2023

davidpdrsn commented Dec 29, 2023

lz1998 commented Dec 29, 2023

davidpdrsn commented Dec 29, 2023

gabibbo97 commented Jan 7, 2024 •

edited

Loading

rbruenig commented Jul 14, 2024

jplatte commented Sep 20, 2024

lz1998 commented Sep 24, 2024

jplatte commented Sep 24, 2024 •

edited

Loading

jplatte commented Sep 27, 2024

jplatte commented Sep 28, 2024

HigherOrderLogic commented Oct 1, 2024

jplatte commented Oct 1, 2024

jplatte commented Oct 5, 2024

use afit and remove async-trait #2308

use afit and remove async-trait #2308

Conversation

lz1998 commented Nov 10, 2023 • edited Loading

Motivation

Solution

davidpdrsn commented Nov 10, 2023

jplatte commented Nov 10, 2023

jplatte commented Nov 10, 2023

davidpdrsn commented Nov 10, 2023

lz1998 commented Nov 10, 2023

lz1998 commented Nov 13, 2023

jplatte left a comment

Choose a reason for hiding this comment

jplatte Nov 13, 2023

Choose a reason for hiding this comment

lz1998 Nov 14, 2023

Choose a reason for hiding this comment

lz1998 Nov 14, 2023

Choose a reason for hiding this comment

jplatte commented Nov 13, 2023

jplatte left a comment

Choose a reason for hiding this comment

lz1998 commented Dec 16, 2023

lz1998 commented Dec 29, 2023 • edited Loading

lz1998 commented Dec 29, 2023

davidpdrsn commented Dec 29, 2023

lz1998 commented Dec 29, 2023

davidpdrsn commented Dec 29, 2023

gabibbo97 commented Jan 7, 2024 • edited Loading

rbruenig commented Jul 14, 2024

jplatte commented Sep 20, 2024

lz1998 commented Sep 24, 2024

jplatte commented Sep 24, 2024 • edited Loading

jplatte commented Sep 27, 2024

jplatte commented Sep 28, 2024

HigherOrderLogic commented Oct 1, 2024

jplatte commented Oct 1, 2024

jplatte commented Oct 5, 2024

lz1998 commented Nov 10, 2023 •

edited

Loading

lz1998 commented Dec 29, 2023 •

edited

Loading

gabibbo97 commented Jan 7, 2024 •

edited

Loading

jplatte commented Sep 24, 2024 •

edited

Loading